Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Move all job PROPERTIES to Jenkins Files for Build and Pipeline jobs #2138

Merged
merged 1 commit into from
Sep 7, 2018

Conversation

AdamBrousseau
Copy link
Contributor

@AdamBrousseau AdamBrousseau commented Jun 8, 2018

  • Add Build Discarder
    • Retrieve values from Variable file
  • Add job Parameters
    • Default values for Vendor Params must be set globally
      even if they are blank
  • No Trigger for Build and Pipeline jobs
  • TODO add trigger for PR builds

[skip ci]
Issue #1017
Signed-off-by: Adam Brousseau adam.brousseau88@gmail.com

Depends #2093

@AdamBrousseau AdamBrousseau changed the title WIP: Add build discarder to generic pipelines Add build discarder to pipelines Jun 16, 2018
@AdamBrousseau
Copy link
Contributor Author

@vsebe, @pshipton please review

// Keep last 50 of each PullRequest build
NUM_BUILDS = 50
}
properties([buildDiscarder(logRotator(artifactDaysToKeepStr: '', artifactNumToKeepStr: NUM_ARTIFACTS, daysToKeepStr: DAYS_BUILDS, numToKeepStr: NUM_BUILDS))])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing strategy?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks not to be mandatory

// Keep last 50 of each PullRequest build
NUM_BUILDS = 50
}
properties([buildDiscarder(logRotator(artifactDaysToKeepStr: '', artifactNumToKeepStr: NUM_ARTIFACTS, daysToKeepStr: DAYS_BUILDS, numToKeepStr: NUM_BUILDS))])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you move this code into a fuction in variables-functions then you avoid duplicating the code.

@AdamBrousseau
Copy link
Contributor Author

jenkins compile win jdk10

@@ -20,6 +20,16 @@
* SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 OR GPL-2.0 WITH Classpath-exception-2.0 OR LicenseRef-GPL-2.0 WITH Assembly-exception
*******************************************************************************/

// Discard old builds
NUM_ARTIFACTS = 5
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move NUM_ARTIFACTS, NUM_BUILDS , DAYS_BUILDS to variables file

Copy link
Contributor

@vsebe vsebe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reformat to use variable files

@AdamBrousseau
Copy link
Contributor Author

jenkins compile win jdk10

1 similar comment
@AdamBrousseau
Copy link
Contributor Author

jenkins compile win jdk10

@AdamBrousseau
Copy link
Contributor Author

jenkins compile win jdk10

3 similar comments
@AdamBrousseau
Copy link
Contributor Author

jenkins compile win jdk10

@AdamBrousseau
Copy link
Contributor Author

jenkins compile win jdk10

@AdamBrousseau
Copy link
Contributor Author

jenkins compile win jdk10

@AdamBrousseau
Copy link
Contributor Author

AdamBrousseau commented Jun 19, 2018

Unfortunately having the properties step will overwrite the manually configured trigger property in the job even if that section isn't defined in the pipeline. I.e. We are trying to add build discarder but since trigger is also a property and its not defined in the code, the PRs trigger section will get obliterated upon the first run of the build.

This issue comment and the next 3 comments sum it up nicely.

See pipeline-syntax Sample Step: properties to see all the properties you can define. We currently use parameters, triggers, and build discarder. Since we set a different SPEC & PLATFORM parameter on every job, I don't this this will work as is.

We might be able to define everything in the jenkinsfiles. We would have to have all the parameters and all the trigger regexes and the user whitelist in the variable file, which is a good thing. Upon initial setup you could leave all the config blank, and we could have an if in the pipeline that first checked for PLATFORM and SPEC, if these were undefined we could parse the job name(#1396) to set them up. Then all the rest of the properties could be determined afterwards.

I will change this PR to WIP and update the title to reflect the larger task.

@AdamBrousseau AdamBrousseau changed the title Add build discarder to pipelines WIP: Move all job PROPERTIES to Jenkins Files Jun 19, 2018
@AdamBrousseau AdamBrousseau force-pushed the add_build_discarder branch 9 times, most recently from 7baa8d7 to ed3aa83 Compare July 27, 2018 19:21
@AdamBrousseau
Copy link
Contributor Author

Jenkins copyright check

@AdamBrousseau AdamBrousseau force-pushed the add_build_discarder branch 8 times, most recently from 051c4ab to a1511a0 Compare September 4, 2018 19:39
/* OPENJDK_REPO, OPENJDK_BRANCH, OPENJDK_SHA,
* OPENJ9_REPO, OPENJ9_BRANCH, OPENJ9_SHA,
* OMR_REPO, OMR_BRANCH, OMR_SHA,
* VARIABLE_FILE, VENDOR_REPO, VENDOR_BRANCH, VENDOR_CREDENTIALS_ID */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could use a for loop


// If SDK_VERSION parameter is already set, assume it is correct (helps with Sandbox builds)
// Else parse the JOB_NAME to determine the verison
if (params.SDK_VERSION) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace if-else with if statement:

SDK_VERSION = params.SDK_VERSION
if (!SDK_VERSION) {
    //infer from job name
    ...
}

}
echo "SDK_VERSION:'${SDK_VERSION}'"

if (params.PLATFORM) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

replace if-else with if statement only

PLATFORM = params.PLATFORM
if (!PLATFORM) { ... }

VENDOR_REPO = params.VENDOR_REPO
VENDOR_CREDENTIALS_ID = params.VENDOR_CREDENTIALS_ID
if (VARIABLE_FILE && VENDOR_REPO && VENDOR_BRANCH) {
echo "VARIABLE_FILE:'${VARIABLE_FILE}'"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

single quotes not needed in echo statements

echo "VENDOR_REPO:'${VENDOR_REPO}'"
echo "VENDOR_BRANCH:'${VENDOR_BRANCH}'"
echo "VENDOR_CREDENTIALS_ID:'${VENDOR_CREDENTIALS_ID}'"
if (VENDOR_CREDENTIALS_ID != '') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (VENDOR_CREDENTIALS_ID != '') replace with if (VENDOR_CREDENTIALS_ID)

// PLEASE NOTE: The following 4 DEFAULT variables must be configured in the Jenkins Global Config (even if they are set to blank).
// This allows us to know what the default values are without being told explicitly.
// These DEFAULT values are typically constant per Jenkins Server.
PARAMETERS.add(string(name: 'VARIABLE_FILE', defaultValue: "${VARIABLE_FILE_DEFAULT}", description: '', trim: true))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

add checks for the *_DEFAULT variables, e.g

if (VARIABLE_FILE_DEFAULT) {
  PARAMETERS.add(string(name: 'VARIABLE_FILE', defaultValue: "${VARIABLE_FILE_DEFAULT}", description: '', trim: true))
}

@AdamBrousseau AdamBrousseau force-pushed the add_build_discarder branch 4 times, most recently from e0b99a5 to 8fafa7b Compare September 6, 2018 20:53
@AdamBrousseau
Copy link
Contributor Author

@smlambert for final review

Copy link
Contributor

@smlambert smlambert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than minor typo, LGTM

if (JOB_TYPE == "build" || JOB_TYPE == "pipeline") {
// Start with just supporting Build and Pipeline jobs
// PR jobs will need some playing to get the trigger figured out
// Also regaring PR builds, see OpenJ9 Issue #2728
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

spelling regaring -> regarding

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

- Add Build Discarder
  - Retrieve values from Variable file
- Add job Parameters
  - Default values for Vendor Params must be set globally
    even if they are blank
- No Trigger for Build and Pipeline jobs
- TODO add trigger for PR builds

Issue eclipse-openj9#1017
[skip ci]
Signed-off-by: Adam Brousseau <adam.brousseau88@gmail.com>
@smlambert smlambert merged commit b9aecfb into eclipse-openj9:master Sep 7, 2018
AdamBrousseau added a commit to AdamBrousseau/openj9 that referenced this pull request Sep 10, 2018
- Using params.PARAM will allow null to be passed
  to a downstream job. This causes a failure if the
  downstream job has the parameter configured.
- Null value not allowed as an environment variable
- Adding a check and setting the variable to blank
  should resolve this failure

[skip ci]
Issue eclipse-openj9#2138

Signed-off-by: Adam Brousseau <adam.brousseau88@gmail.com>
AdamBrousseau added a commit to AdamBrousseau/openj9 that referenced this pull request Sep 26, 2018
- Add BUILD_NODE, TEST_NODE, PERSONAL_BUILD and SLACK_CHANNEL
- Missed in original PR eclipse-openj9#2138

[skip ci]

Signed-off-by: Adam Brousseau <adam.brousseau88@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants